Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(db): Prevent two connections for single node databases #45013

Merged
merged 1 commit into from
May 28, 2024
Merged

fix(db): Prevent two connections for single node databases #45013

merged 1 commit into from
May 28, 2024

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Apr 24, 2024 •
edited
Loading

Summary

Two connections to one database node

Since #41998 our connection always extends Doctrine's PrimaryReadReplicaConnection. This baseclass requires a primary and at least one replica and always does the read-write split.
For simpler Nextcloud installations we only have one database node and no replica. We do not want two database connections.
This overwrites the \Doctrine\DBAL\Connections\PrimaryReadReplicaConnection::performConnect method so we always connect to the primary if there is no replica. If there is one, we do the Doctrine read-write split.

Missing transaction isolation level for replica

Nextcloud uses READ COMMITTED as transaction isolation level. Setting it for the session was missing for the replica connections. Reverted because it implicitly causes a connection to the primary just to set the session variable.

How to test - Single node

  1. Set up nextcloud with a single database node (mysql, postgres, oracle)
  2. Enable query logging
    I. 'log_query' => true',
    II. 'query_log_file' => '/var/www/nextcloud/data/query.log',
  3. Send some requests

master: you see replica and primary connections prefixes for the logged queries. That indicates that Nextcloud opens two connections per request.
here: you only see primary connections. That indicates that Nextcloud only opens one connection per request.

How to test - Cluster

  1. Set up nextcloud with a single database node (mysql, postgres, oracle)
  2. Add a fake replica by using a different hostname
  3. Enable query logging
    I. 'log_query' => true',
    II. 'query_log_file' => '/var/www/nextcloud/data/query.log',
  4. Send some requests

master and here: Nextcloud opens two connections.

Checklist

@ChristophWurst ChristophWurst self-assigned this Apr 24, 2024
@ChristophWurst ChristophWurst changed the title Fix/db/two primary connections transaction isolation level fix(db): Two primary connections and missing transaction isolation level Apr 24, 2024
@ChristophWurst
Copy link
Member Author

The downside of this approach is that the query logging will speak the truth for single database node setups and prefix queries with "primary" even if they would go to a replica on clustered setups.

@ChristophWurst
Copy link
Member Author

/backport to stable29

@ChristophWurst
Copy link
Member Author

The downside of this approach is that the query logging will speak the truth for single database node setups and prefix queries with "primary" even if they would go to a replica on clustered setups.

This feels tricky to achieve without breaking too much. The current approach works for single node and clustered databases. A unit test was added to ensure this doesn't happen again.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 24, 2024
@ChristophWurst ChristophWurst marked this pull request as ready for review May 24, 2024 09:48
@ChristophWurst ChristophWurst added this to the Nextcloud 30 milestone May 24, 2024
@ChristophWurst
Copy link
Member Author

I have reverted the second part of setting the transaction isolation level for read replicas.

This PR now only fixes having two connections for one request connecting to a single node database.

@ChristophWurst ChristophWurst changed the title fix(db): Two primary connections and missing transaction isolation level fix(db): Prevent two primary connections May 27, 2024
@ChristophWurst ChristophWurst changed the title fix(db): Prevent two primary connections fix(db): Prevent two connections for single node databases May 27, 2024
@kesselb
Copy link
Contributor

kesselb commented May 27, 2024

Let's try to rewrite this code sometime and make our connection class a decorator for the doctrine classes rather than extending it.

@ChristophWurst ChristophWurst force-pushed the fix/db/two-primary-connections-transaction-isolation-level branch from b735389 to fe94824 Compare May 28, 2024 07:39
@ChristophWurst
Copy link
Member Author

Squashed

artonge
artonge reviewed May 28, 2024
View reviewed changes
tests/lib/DB/ConnectionTest.php
'host' => 'test',
];
$adapter = $this->createMock(Adapter::class);
$driver = $this->createMock(Driver::class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You apparently need to mock detectDatabasePlatform

https://github.com/nextcloud/server/actions/runs/9265386150/job/25487375754?pr=45013

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by passing an actual platform. Thanks!

@ChristophWurst
fix(db): Prevent two connections for single node databases
3bfba20 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/db/two-primary-connections-transaction-isolation-level branch from fe94824 to 3bfba20 Compare May 28, 2024 08:38
@ChristophWurst ChristophWurst merged commit 4a3bb05 into master May 28, 2024
155 checks passed
@ChristophWurst ChristophWurst deleted the fix/db/two-primary-connections-transaction-isolation-level branch May 28, 2024 09:44
@backportbot backportbot bot mentioned this pull request May 28, 2024
Merged
This was referenced May 31, 2024
Merged
Open
@szaimen
Copy link
Contributor

szaimen commented May 31, 2024

Hi, is this fixing the problem #45257?

@ChristophWurst
Copy link
Member Author

I don't think so

@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge left review comments

@Altahrim Altahrim Altahrim approved these changes

@joshtrichards joshtrichards Awaiting requested review from joshtrichards

@blizzz blizzz Awaiting requested review from blizzz

@come-nc come-nc Awaiting requested review from come-nc

@kesselb kesselb Awaiting requested review from kesselb

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review Waiting for reviews bug regression
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

[Bug]: Two database connections and missing transaction isolation level for read operations
5 participants
@ChristophWurst @kesselb @szaimen @Altahrim @artonge